Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KIC: Cleanup of old named resources #1819

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Conversation

oshoval
Copy link
Collaborator

@oshoval oshoval commented Jul 2, 2024

What this PR does / why we need it:
Since Kubevirt ipam controller resource names were renamed [1]
since 0.94.0, we introduce the special remove that in case the old named
objects exist, will remove them.
This allows the lifecycle lane to pass, as it checks for leftovers,
which otherwise exists.

[1] #1816

Special notes for your reviewer:

Release note:

None

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jul 2, 2024
@kubevirt-bot kubevirt-bot requested review from qinqon and RamLavi July 2, 2024 06:40
@kubevirt-bot kubevirt-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. release-note-none Denotes a PR that doesn't merit a release note. size/M and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 2, 2024
@oshoval oshoval mentioned this pull request Jul 2, 2024
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 2, 2024

https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_cluster-network-addons-operator/1819/pull-cluster-network-addons-operator-unit-test/1808027843397947392

seems it fails due to something like this
operator-framework/operator-registry#178 (comment)

so we would need to use deselector
or to remove the replacement field as the link above says (trying this first, it works fwiw)
EDIT - we can also instead removing the field, point to previous release

@oshoval
Copy link
Collaborator Author

oshoval commented Jul 2, 2024

/cc @RamLavi
ptal

@RamLavi
Copy link
Collaborator

RamLavi commented Jul 2, 2024

Not sure what you mean by "Delete release 0.94.0"
It's a version, U/S user may already consume it.
Can we just remove support from it? it breaks the entire lifecycle

@oshoval
Copy link
Collaborator Author

oshoval commented Jul 2, 2024

it is very new and not released beside on upstream main
i think it is safe to delete it, we did that in the past

if we don't delete it lifecycle fails #1818 (comment)

since the kubevirt ipam controller component name was changed it leaves leftovers (details in the link above)
the alternative is to add it to deselector
imo it is better and safe to delete

@RamLavi
Copy link
Collaborator

RamLavi commented Jul 2, 2024

it is very new and not released beside on upstream main i think it is safe to delete it, we did that in the past

if we don't delete it lifecycle fails #1818 (comment)

since the kubevirt ipam controller component name was changed it leaves leftovers (details in the link above) the alternative is to add it to deselector

It is not permitted to remove a supported release. Once it is tagged - you can't go back.
From what I see - CNAO API wasn't changed - am I correct?
Regarding the now "old" manifests - CNAO needs to handle them, IIRC we have a special place for that. I suggest we go towards that approach
/hold

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2024
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 2, 2024

it is very new and not released beside on upstream main i think it is safe to delete it, we did that in the past
if we don't delete it lifecycle fails #1818 (comment)
since the kubevirt ipam controller component name was changed it leaves leftovers (details in the link above) the alternative is to add it to deselector

It is not permitted to remove a supported release. Once it is tagged - you can't go back. From what I see - CNAO API wasn't changed - am I correct? Regarding the now "old" manifests - CNAO needs to handle them, IIRC we have a special place for that. I suggest we go towards that approach /hold

CNAO API wasn't change, correct.

This is a release on main branch which is unstable
Having a function like cleanUpMultusOldName will need to delete a list of resources (as they all were changed)
I think it is a total overkill in this case.

lets at least do the deselect and say remove support in release notes?
if any user uses 0.94.0 we can just request to clean and reinstall one time
again it is a release on main branch, unstable, and the probability someone already uses it and will have problems is rare
(at most there is just release notes)
Dragging logic like cleanUpMultusOldName which will be here even more complex is wrong imho
in this case.

Anyhow if you insist i will just do it, honestly it is overkill imho

@RamLavi
Copy link
Collaborator

RamLavi commented Jul 2, 2024

lets at least do the deselect and say remove support in release notes?

I disagree. Every release should be supported lifecycle-wize. main branch is theoretically unstable but it doesn't mean we get to break the lifecycle. This is precisely the reason the lane is there for.
I am not convinced that this case justify breaking the lane.

@oshoval
Copy link
Collaborator Author

oshoval commented Jul 2, 2024

lets at least do the deselect and say remove support in release notes?

I disagree. Every release should be supported lifecycle-wize. main branch is theoretically unstable but it doesn't mean we get to break the lifecycle. This is precisely the reason the lane is there for. I am not convinced that this case justify breaking the lane.

we even didn't even finish delivering the feature, the release was created only so HCO can consume
and we renamed the repo as soon as possible, which caused this.
adding this removal function in so early stage consumes focus and reduce simplification imho
(only one release is dropped)
being robust to changes here is the right thing imho
unstable branch should not be committed the the same factors as stable ones

anyhow as mentioned above, will just do that

@oshoval
Copy link
Collaborator Author

oshoval commented Jul 2, 2024

Done (added special remove), lets see it pass (didn't try locally)
maybe this new commit can be added as a prior PR to the release

@oshoval oshoval changed the title [main] Release v0.94.1 WIP [main] Release v0.94.1 Jul 2, 2024
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 2, 2024
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 2, 2024

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2024
@oshoval oshoval changed the title WIP [main] Release v0.94.1 WIP [main] Release v0.94.1 (+cleanup of old named KIC resources) Jul 2, 2024
@oshoval oshoval changed the title WIP [main] Release v0.94.1 (+cleanup of old named KIC resources) [main] Release v0.94.1 (+cleanup of old named KIC resources) Jul 2, 2024
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 2, 2024
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 2, 2024

@RamLavi ptal

if needed i can split this PR to 2 different PRs
one is the fix and then comes the release
i think it should work as well

@oshoval oshoval marked this pull request as draft July 3, 2024 06:26
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2024
@oshoval oshoval changed the title [main] Release v0.94.1 (+cleanup of old named KIC resources) KIC: Cleanup of old named resources Jul 3, 2024
@oshoval oshoval marked this pull request as ready for review July 3, 2024 06:29
@kubevirt-bot kubevirt-bot added size/M and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL labels Jul 3, 2024
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 3, 2024

split the commits, this PR will be just the cleanup
and then we would able to take #1818
(both PRs were tested together already)

@oshoval
Copy link
Collaborator Author

oshoval commented Jul 3, 2024

workflow fix
#1820

@RamLavi
Copy link
Collaborator

RamLavi commented Jul 3, 2024

@RamLavi ptal

if needed i can split this PR to 2 different PRs one is the fix and then comes the release i think it should work as well

why do we need to split? this commit has no place but after a new release was created. Am I missing something?

@oshoval
Copy link
Collaborator Author

oshoval commented Jul 3, 2024

@RamLavi ptal
if needed i can split this PR to 2 different PRs one is the fix and then comes the release i think it should work as well

why do we need to split? this commit has no place but after a new release was created. Am I missing something?

the other way around
the renaming was already merged, so this is the cleanup
and then a new release can be introduced
it is better that new releases are PRs which are automated once possible
the PR can be standalone, it is not that they depend and need to be bundled
they just both need to be merged
the fact the logic is there doesnt check what release are we on, because the renaming is already included
anyhow it is really nits imho

@RamLavi
Copy link
Collaborator

RamLavi commented Jul 3, 2024

@RamLavi ptal
if needed i can split this PR to 2 different PRs one is the fix and then comes the release i think it should work as well

why do we need to split? this commit has no place but after a new release was created. Am I missing something?

the other way around the renaming was already merged, so this is the cleanup and then a new release can be introduced it is better that new releases are PRs which are automated once possible the PR can be standalone, it is not that they depend and need to be bundled they just both need to be merged the fact the logic is there doesnt check what release are we on, because the renaming is already included anyhow it is really nits imho

oh I see then.

@oshoval
Copy link
Collaborator Author

oshoval commented Jul 3, 2024

/override pull-e2e-cluster-network-addons-operator-workflow-k8s

@kubevirt-bot
Copy link
Collaborator

@oshoval: Overrode contexts on behalf of oshoval: pull-e2e-cluster-network-addons-operator-workflow-k8s

In response to this:

/override pull-e2e-cluster-network-addons-operator-workflow-k8s

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link
Collaborator

@RamLavi RamLavi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good! just needs to move to the right location

pkg/network/multus.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@RamLavi RamLavi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed something: please add the PR that renamed the objects on the commit desc.

Since Kubevirt ipam controller resource names were renamed [1]
since 0.94.0, we introduce the special remove that in case the old named
objects exist, will remove them.
This allows the lifecycle lane to pass, as it checks for leftovers,
which otherwise exists.

[1] kubevirt#1816

Signed-off-by: Or Shoval <[email protected]>
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 3, 2024

done

Copy link
Collaborator

@RamLavi RamLavi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2024
@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RamLavi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2024
@kubevirt-bot kubevirt-bot merged commit 3596109 into kubevirt:main Jul 3, 2024
15 checks passed
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 3, 2024

btw all those cleanups (not just this one)
becomes outdated at some stage and can be removed
it also cause API calls, maybe we can consider a boolean that once the flow saw that nothing should be removed (but conf.ipam == true), or all that was to removed, removed successfully, short circuit the function
unless those API calls arent expansive

alternative is to not scan for this type of leftovers (but i guess it is less nice solution)

@RamLavi
Copy link
Collaborator

RamLavi commented Jul 3, 2024

btw all those cleanups (not just this one) becomes outdated at some stage and can be removed it also cause API calls, maybe we can consider a boolean that once the flow saw that nothing should be removed (but conf.ipam == true), or all that was to removed, removed successfully, short circuit the function unless those API calls arent expansive

alternative is to not scan for this type of leftovers (but i guess it is less nice solution)

The same logic needs to apply like the nmstate tests - if it should no longer be supported then we can remove the special clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants